In the spirit of learning and spreading knowledge, I have chosen @jhiesey and @raulk as reviewers of this PR.

We should probably say: Implementations SHOULD support RSA and Ed25519. Implementations MAY support Secp256k1 and ECDSA but nodes using those keys may not be able to connect to all nodes.

+1 for using normative language

Any update here? I'd love to have some specs around here to link to ;)

@yusefnapora if you wanna do some spec herding, this is a quick win I think. Pretty good consensus.

I did a quick edit to remove references to private keys and serialization on disk. I also removed links to go code and added some links to e.g. the RSA signing spec, etc.

@mgoelzer do you mind if I push changes to this branch? I put up a new one here with the edits: https://github.com/libp2p/specs/blob/edit/peer-ids/peer-ids/peer-ids.md

but it might be easier to discuss if I drop the commits here.

@yusefnapora gonna jump in and say yes. In the interest of moving forward, push to the branch in this PR. Thanks!

Sorry for sleeping on this for a while everyone :)

I added a few commits to address some feedback. I think the most significant is the note about deterministic protobuf encoding. It basically says determinism is "desirable" and you should try to make it happen, but doesn't call it out as a MUST. Without requiring a bunch of changes to the protobuf spec, that might be the best we can do, but if anyone has a better way to put this, I'm all ears.

@Stebalien @arnetheduck @raulk - could you guys help me figure out the resolution to the deterministic encoding problem?

If we definitely want to require a consistent / canonical encoding for peer ids, then I think I should write up a precise spec that requires a certain field ordering, etc. And we can have some tests that ensure your encoder handles edge cases well.

But @Stebalien mentions that, because we're also not guaranteeing a canonical encoding for the key Data field, it's not really relevant at this layer. And that it's fine to have multiple valid peer ids for the same key, which seems contentious.

I can make up some arguments in favor of this view; for one, we can extend the PublicKey message in the future without having to decide now how to encode unknown fields in our "not-quite-protobuf" format. It could also be up to an application to decide whether to reject multiple peer ids that all derive from the same public key. It's also much simpler, of course, since the easiest problem to solve is the one you don't have.

@Stebalien could you elaborate a bit on your view? I think we should figure out if this is a blocker or not to merging the spec.

On mobile.

Re: deterministic key serde. I suggest we specify the format as proto3 + the extra requirements to reach a deterministic result (ordering, no unrecognised fields, no duplicates last wins, etc.) We should add an implementers note in the form of a SHOULD recommendation to use an OOTB protobuf encoder, where possible, and provide test vectors.

For cases where that’s unfeasible, we should provide a boiled down serde spec in BNF or similar form. It’ll be super simple, the schema is so short and constrained we can express the serialised form manually without alluding to proto3 at all.

Re: using unrecognised fields for peer ID calculation, I’d like to hear what use cases you had in mind @Stebalien. In my view, the peer ID should be derived from the pubkey modulo metadata, if any. I don’t think user-defined metadata should yield a different identity. Seems like opening a trivial attack vector for sybils.

In my view, the peer ID should be derived from the pubkey modulo metadata, if any. I don’t think user-defined metadata should yield a different identity.

Note: I'm really not expecting much if any user defined metadata. However, we may want to add new fields in the future and it's hard to do this if we don't include them in the hash.

1. We (IPFS/IPLD) would like to to be able to convert peer IDs to CIDs (and, ideally, fetch keys over bitswap).
2. Currently, peer IDs always map to exactly one public key. If we both allow other metadata and don't hash it, we'll need to handle merging metadata.
3. The un-hashed metadata won't be authenticated. When we connect to a peer, they send us their key. We then compute the peer ID of that key and check if it matches the expected one. If we ignore metadata, we could end up with inconsistent metadata views.

I don’t think user-defined metadata should yield a different identity. Seems like opening a trivial attack vector for sybils.

There are two cases:

• The key's owner generates multiple identities. This is a non-issue, they can already generate more keys.
• An attacker generates multiple identities for a victim: We'll detect this on connect because we'll compute the victim's peer ID from the key they give us. Ideally, keys would be self signed but our use-case for these keys is simple enough that this probably isn't that big of an issue (we should keep using this system for identifying libp2p peers but should come up with something better for identity).

But @Stebalien mentions that, because we're also not guaranteeing a canonical encoding for the key Data field, it's not really relevant at this layer. And that it's fine to have multiple valid peer ids for the same key, which seems contentious.

Same Key

IMO, "same key" should mean "same bytes". That is, if I change anything about the bytes of the serialized key, I get a new key.

I'm concerned that not all key formats will have a "canonical" encoding, some libraries may strip certain metadata while others preserve it, etc. This will lead to hard-to-track-down bugs. This has already been a real pain for us in IPLD and our solution there is to avoid re-serializing unless we change something.

My unconcern with having multiple valid peer IDs for the same underlying cryptographic key is that there's likely nothing we can do to stop this. I haven't audited our key formats/algorithms but it's likely possible to make some small changes to a public key and have it continue to work with the private key.

@raulk, what do you think of the header in 6c4a587 - I think the markdown table is a decent way to present the main status information.

I figure we can hammer out details here and I'll write up a little spec for the header format once we like it.

Also, @raulk, @vyzo & @Stebalien I nominated you guys as the Interest Group for this one 😄

Others are welcome to join in if they like

I would like to reiterate the value in migrating to embedded-key base32 encoded canonical representation of PeerIDs
Ref. #139

How do we feel about merging this?

870b71a kicks the deterministic encoding issue down the road by saying a future version of the spec may require more strict encoding than the protobuf spec, but until then, don't extend the PublicKey message and use a protobuf encoder that writes fields in order.

@mgoelzer @Stebalien @raulk @vyzo

Sorry for commenting on an old PR, but I have not found an answer anywhere else.

What's the motivation for number 42 here?

keys that serialize to at most 42 bytes must be hashed using the "identity" multihash codec.

When one would expect keys to be longer than 42 bytes?

Thank you!

I believe it's the size of ed25519 public keys inside of protobuf encoding, since ed25519 keys are supposed to be inlined into PeerIDs, so any keypair algorithm with public keys larger than 32 bytes would be encoded as more than 42 bytes and therefore not be inlined

uh, so it's 32 + 10 bytes.

30% for serializaton is such a big number, I didn't even think about that.

thank you, that makes it clearer🙏

And I vaguely remember that 42 would be small enough to then fit into a single DNS segment. Though I might be misremembering that.